feat: Add braintrust/apply-instrumentation entrypoint for CJS/TS patching#2038
feat: Add braintrust/apply-instrumentation entrypoint for CJS/TS patching#2038Luca Forstner (lforst) wants to merge 4 commits into
braintrust/apply-instrumentation entrypoint for CJS/TS patching#2038Conversation
Luca Forstner (lforst)
requested review from
Abhijeet Prasad (AbhiPrasad) and
Stephen Belanger (Qard)
…ntation # Conflicts: # js/src/auto-instrumentations/configs/all.ts # js/src/instrumentation/braintrust-plugin.ts # js/src/instrumentation/registry.ts
| "braintrust": minor | ||
| --- | ||
|
|
||
| feat: Add `braintrust/apply-instrumentation` entrypoint for CJS/TS patching |
There was a problem hiding this comment.
should we call it braintrust/apply-auto-instrumentation instead?
Stephen Belanger (Qard)
left a comment
Stephen Belanger (Qard)
left a comment
There was a problem hiding this comment.
This will likely be fairly unreliable. The reason we use --import rather than a first import is that import execution order is non-deterministic. It does not happen by order of appearance in the graph, it happens by load completion order.
ESM will load all imports in parallel recursively and will execute in the order in which modules complete loading and either have their dependencies already executed or have no dependencies. So it runs from the leaf nodes in load completion order and makes its way back to the root. This means that by the time it gets back to the entrypoint root node a later import in that file could be fully resolved before an earlier one depending on which was able to load its sub-graph faster.
With --import it will fully resolve the sub-graph of that file before beginning the resolve of the entrypoint file.
I won't block landing this because it might work for some users, but we should be aware that the ordering of this will be inconsistent and may result in users complaining about some imports not getting patched sometimes.
Afaik yes, but this is not what I care about with this PR. IIRC applying the esm hooks before doing a dynamic import as I've outlined in the PR will always apply the hooks to the dynamically imported module. I was going for solving the dynamic imports case and the I do not think there currently is a solution to patching "normal" imports via normal runtime code. |
|
Ah, yes, a dynamic import graph resolution will happen later, so that could be fine. 👍🏻 |
3 participants
We have the problem that we don't have a good setup story around frameworks like Nestjs that are CJS but don't give you a clean way of passing the
--importhook. For this I thought we could expose something likeimport 'braintrust/apply-instrumentation'which is simply an import with a side effect that applies runtime patching. It's also good for platforms like Vercel where passing the--importargument is super painful in general.This will now be usable with
require('braintrust/apply-instrumentation')for all subsequentrequiresimport 'braintrust/apply-instrumentation'for all subsequent dynamic imports (import('foobar').then(() => console.log('foobar is instrumented')))initLoggercan be called whenever.Currently we only expose this for Nodejs - I haven't looked into how to make this work for other frameworks. I generally like the ergonomics of this and it is an additional tool in our toolbox of doing instrumentation.